Skip to content

Tigers - Renee Semhar#20

Open
reneesu99 wants to merge 21 commits intoAda-C18:mainfrom
reneesu99:main
Open

Tigers - Renee Semhar#20
reneesu99 wants to merge 21 commits intoAda-C18:mainfrom
reneesu99:main

Conversation

@reneesu99
Copy link
Copy Markdown

Copy link
Copy Markdown

@CheezItMan CheezItMan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work Renee & Semhar, you hit the learning goals here. Well done!

Comment thread app/models/planet.py
Comment on lines +11 to +26
def to_dict(self):
return {
"name": self.name,
"description": self.description,
"moons": self.moons,
"id": self.id,
}

@classmethod
def from_dict(cls, planet_data):
planet_1 = Planet(
name=planet_data["name"],
description=planet_data["description"],
moons=planet_data["moons"]
)
return planet_1 No newline at end of file
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 nice helper functions

Comment thread app/planets.py
@@ -0,0 +1,50 @@
# from flask import jsonify, abort, make_response
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can probably delete this file

Comment thread app/routes.py
@planet_bp.route("", methods=["POST", "GET"])
def handle_planets():
if request.method == "POST":
request_body = request.get_json()
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing some validation here on the request body would be appropriate (make sure it has the required fields).

Comment thread app/routes.py
planets = Planet.query.filter_by(moons = moons_param)
else:
planets= Planet.query.all()
response_body = [planet.to_dict() for planet in planets]
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the list comprehension here and use of the to_dict method.

Comment thread app/routes.py
Comment on lines +27 to +37
def validate_id(id, cls):
try:
id = int(id)
except:
abort(make_response ({"Message": f"{cls.__name__} {id} invalid."}, 400))

# to access a planet with planet_id in our db, we use
obj = cls.query.get(id)
if not obj:
abort(make_response({"Message": f"{cls.__name__} {id} not found."}, 404))
return obj
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good helper

Comment thread app/routes.py
return planet.to_dict()

elif request.method == "PUT":
request_body = request.get_json()
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validating the request body would be good here.

Comment thread tests/test_routes.py
"description": "i luv storms"
}

def test_get_query_param_planets(client, two_saved_planets):
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great test on the query params!

Comment thread tests/test_routes.py



def test_post_planet(client):
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also testing a post request with an invalid request body would be great.

Comment thread tests/test_routes.py
# Assert
assert response.status_code == 201

def test_update_planet(client, two_saved_planets):
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also testing a put request with an invalid request body would be great.

You should also test a put request with an invalid/missing planet id.

Comment thread tests/test_routes.py
Comment on lines +92 to +100
def test_delete_planet(client, two_saved_planets):
# Act
response = client.delete("/planets/1")
assert response.status_code == 200

def test_delete_planet_invalid_id(client, two_saved_planets):
# Act
response = client.delete("/planets/random")
assert response.status_code == 400
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants